-
-
Notifications
You must be signed in to change notification settings - Fork 701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
std/path: make globMatch work with @nogc #9055
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @ljmf00-wekaio! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#9055" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise looks good
f0085e8
to
ec0ae81
Compare
@jmdavis Brought this to my attention. I think this code points out a fundamental flaw in how we use This is because in D, Of course, we can always fall-back to "you should just know that The overarching problem here is that As this code fails to meet the Phobos 3 design standards I would request that you either redesign this method to meet them or consider providing an alternative implementation via overload. |
Calling a function allocates memory (on the stack). I think you would need to define what "allocate memory" means more precisely before beginning to discuss whether
Actually that is not quite correct. |
That's a valid point. We should try to be precise with what we mean. However, I think that it's pretty clear that he meant heap allocations, which is almost always what people mean when talking about allocations. Stack allocations happen all over the place whether you like it or not and aren't avoidable to the point that there's really no point in even discussing them with regards to APIs. The fact that they happen is a given, and they're a non-issue unless you start doing something like creating huge static arrays or run attempt really deep / infinite recursion.
From the standpoint of actually using |
No, it's conceptually wrong and I don't think the community follows that either. The whole reason for GC being costly is because of the entire mechanism of tracking the memory, not the allocation mechanism alone. The same for nothrow but with the argument that the exception unwinding code is expensive.
I think those are mostly your assumptions, at least in current Druntime/Phobos. I can prove that with a simple grep in the code. See phobos/std/algorithm/sorting.d Line 3188 in 59b3715
Take another example, where user assumes X or Y method is pretty fast, but turns out the method does a syscall in the background? What about a specific syscall that is potentially more expensive to the user? I think its not sensible to assume that the method does or does not a syscall nor a specific one, unless its documented properly, and I guess the same goes with allocations and other specific behaviour. Would be cool to have rules for this, like enforce documenting it? Yes, do we have it currently? No. Can we still document it, yes.
It's literally the meaning of
While I agree with the allocations (we need to redesign the current Phobos allocators tho), why you bringing this PR, which changes a Phobos v2 implementation?
Sorry, but you seem to fabricate the second meaning. There's only one meaning of the
Why are we trying to make Phobos v2 the Phobos v3 already? We can clearly see this has room for debate, but I don't understand why here. The effort here is to avoid calling a GC collection for such trivial method. I could rework the method to not even use any sort of allocation, which, btw, I think that is the most sensible way, but that's beyond this change. This change itself is an improvement over the previous situation. |
// Allocate this only once per function invocation. | ||
// Should do it with malloc/free, but that would make it impure. | ||
pattmp = new C[pattern.length]; | ||
pattmp = (() @trusted => (cast(C*)pureMalloc(C.sizeof * pattern.length))[0 .. pattern.length])(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enforce space after cast(...)
grep -nrE '[^"]cast\([^)]*?\)[[:alnum:]]' $(find etc std -name '*.d') ; test $? -eq 1
std/path.d:3490: pattmp = (() @trusted => (cast(C*)pureMalloc(C.sizeof * pattern.length))[0 .. pattern.length])();
I think that we should stop considering that @nogc code is synonymous to 'extremely fast code' (and randomly add restrictions like "no allocations" to its meaning on that basis). You can be @nogc and still have expensive calls to other @nogc functions. @nogc functions simply mean that this function does not allocate using the gc (which by the way, is not that expensive in most situations), however, other functions that call the @nogc function can still use the gc. I don't think that there is a guarantee that @nogc guards you against a "stop the world" event if functions up the call stack have allocated gc memory (but I might be wrong). Anyway, my point is that if performance is what you are interested in, then the best way to do it is to start by using the gc, profile and see where the bottlenecks are. It might be that the gc is not where the problem is. This @nogc misconception only propagates gcphobia, when in fact a better design would be to have the possibility of selecting from a wide range of available gc's, including custom ones. I suspect that that will make gc more attractive for more people. And if your mind is set to not use the gc then you might as well compile with -betterC now that most hooks are templated. |
Thank you for raising this. I think this is not even consensual among the community either. And I'm pretty sure, we, at Weka, doesn't consider The boundaries are so vast that we would have to imply an extensive list of rules. Just by saying that To complete some of the Weka concerns on Phobos: @LightBender There's implementations that are supposed to be @nogc, but we don't use from Phobos, e.g.
In our case, using GC, regardless of being a small or large allocation, its potentially costly. I think with the refactor presented in dconf of the GC, this might be way better, but, mainly:
Other reasons to avoid the current GC implementation and some Phobos implementations, for very specific reasons for us, like, but not limited, usage of huge pages, we also need to forbid all kinds of voluntary context switch when possible or usage of Allocations via All of those, in a hot path is costly, for some more than others. |
@RazvanN7 Nominally, I agree. However, we have a perception problem with @ljmf00-wekaio Now lets address your concerns. First. If a non-allocating implementation exists, why not use that? Second. The reason to do this to Phobos 3 standards is simple. We're going to have to do it eventually when we pull this code into V3. So why not do it now? This code hits a key V3 design standard on allocations and there is no harm in either rewriting it to not allocate or adding an overload that takes in a buffer per the V3 standard. Lastly. Consider that the new GC is going to hijack malloc/free so that it can track malloc'd memory in the C standard library. Therefore calling malloc actually will be the same as calling This is why the Phobos V3 design standard is "no/reduced allocations" not "@nogc". Allocation/deallocation runtime performance is in fact undefined from the perspective of the standard library. Honestly, given what is coming with the new GC, and the idea of hijacking malloc/free in general, I think the value of @nogc is extremely limited, to the point where I would advocate removing it from the language entirely. And we're already talking about weakening it to allow allocations when throwing. As far as I know, Weka likely won't care about any of this, because IIRC you guys do other things and don't use the standard GC. Therefore, while this change actually does deliver what you want, in the general case it, in fact, will do something else entirely. Our job with Phobos is to design to that general case. |
Music to my ears. |
No description provided.